iterativerobotpy.py and timedrobotpy.py #161
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
virtuald
reviewed
Apr 6, 2025
| def testExit(self): | ||
| pass | ||
|
|
||
| # todo @Deprecated(forRemoval=true, since="2025") |
There was a problem hiding this comment.
When we deprecate things, we typically only do it in the docstring.
| wpilib.DriverStation.refreshData() | ||
| self._m_watchdog.reset() | ||
|
|
||
| self._m_word.refresh() # todo from Java implementation |
There was a problem hiding this comment.
C++ doesn't have this, I filed a bug to fix that (wpilibsuite/allwpilib#7889).
For now, just do self._word = DSControlWord() and it should have the same effect. Leave a TODO note to switch to refresh when it gets implemented.
There was a problem hiding this comment.
Actually, robotBase has a GetControlState method which returns (enabled, autonomous, test)... which I think would be better to use here.
There was a problem hiding this comment.
@auscompgeek AI tells me that "The eyes emoji (👀) generally indicates a sense of observation, attention, or interest, often with a subtle hint of intrigue or admiration. It can also be used to suggest that someone is being watched or is deliberately drawing attention to something."
What are you thinking here?
| # private void printLoopOverrunMessage() { | ||
| # DriverStation.reportWarning("Loop time of " + m_period + "s overrun\n", false); | ||
| # } | ||
| wpilib._impl.report_error.reportWarning( |
There was a problem hiding this comment.
Yes, this is correct.
I would do at the top of this file from wpilib import reportWarning. Every time there's a . in python it's another dictionary lookup, and the rio is really slow.
auscompgeek
reviewed
Apr 6, 2025
| self.driverStationConnected() | ||
|
|
||
| # If mode changed, call mode exit and entry functions | ||
| if self._m_lastMode != mode: |
There was a problem hiding this comment.
nit: Python enums are singletons, so prefer using is like the recommendation in the Python docs for enum
|
Top level questions: Is the filename right? Is the file in the right spot in the tree? Is the class name right? What are the appropriate tests? What happens if someone deletes the watchdog? Is the there a higher level watchdog that will stop the motors? Can somebody figure out how to call the shuffleboard? |
|
I'm not sure about the filename/classname yet. I think you're approaching it as an alternative, but I sorta want a replacement? But, I guess we could go either way. TBH, before deciding on alternative/replacement, probably need someone to do some testing on a real robot to see if it makes loop overrun issues more/less/same.
... yeah, the only real way to test this is likely going to be by testing examples, which is something I'd like to do but haven't taken the time to do (see #154). Java has tests for TimedRobot, but they basically do the same thing that the pyfrc test framework does (maybe that's an argument for moving some of that into here...).
If someone deletes the watchdog, that's their problem -- but also, the motor watchdog is actually a separate thing? This one just tracks loop overruns and tells the user about it.
|
|
This version simulates a TimedRobot, advances time, logs to AdvantageScope in a reasonable way, but does not move the robot in simulation. It feels like some sort of enable interlock on the simulator is missing. Generally all status results from hal.xxx calls seem to result in a strange value, but the calls seem to work.
do not seem to make the pure python files editable in place. Could use some detailed advice on how to get the python files to be editable in place. |
|
This is the code I'm using to simulate: https://github.com/spiresfrc9106/spiresRobot2025/blob/exp-timedrobotpy/robot.py#L46 |
|
For |
|
It's hard to tell which libraries need to be uninstalled for mostrobotpy and which need to stay for robotpy: t2.txt: |
|
The error you're getting has to do with a quirk of how macOS support is implemented. What it comes down to is that you need to have all robotpy wrapper dependencies installed the same way (either via develop or installed normally). If you mix it then you sometimes get weird errors like that. Typically, I use virtualenv (and specifically, virtualenvwrapper) to manage my various python development environments. I don't recommend that normal RobotPy users use virtualenvs unless they're comfortable with them, but they're really useful for python development -- and because they're effectively disposable, you can do a lot of weird changes in one environment without worrying too much about how much you're breaking. Specifically, I have different environments set up for installation from pypi, development, and various branches that I might be working on. If you were using a virtualenv, I would just say delete your virtualenv and start installing packages from scratch. If you're not using a virtualenv, then just uninstall all robotpy-* packages and wpilib and pyntcore, and then run develop again. |
|
I normally use When I re-ran that in an empty My conclusion is the More basic though is that https://github.com/robotpy/mostrobotpy/blob/main/README.md should define the relationship (what they do and where they are and how to find instructions to build/install them) between pc Also more basic is that https://github.com/robotpy/mostrobotpy/blob/main/README.md should define when to run:
and and and the command to then the installation instructions should take one all the way to a |
Yes and no. As mentioned above:
What this means is that if you are installing the base robotpy packages in develop mode, you must also install robotpy-rev, ctre, etc in develop mode. To install robotpy-rev, you would need to There might be a way out of the macOS problem by messing with the library name, but I haven't had time to look deeper into it. I agree there should be more documentation. I also agree the installer scripts should be better to make this easier. |
|
I have been trying lots of different paths to build mostrobotpy editable on OSX, Debian, and Windows, on a full production robot code base in simulation. Goal is to be able to run on real robot code. (I'm typing not cut and pasting so there may be some typo's here) I end up with two copies of many of the repos needed in the system: I can uninstall those with But then I need to install a specific version of photonlibpy: which will then also uninstalls the editable development parts of robotpy Note that I have been automating these steps here: https://github.com/MikeStitt/pythonExperiments/tree/main/tryRobotPyBuildEditable But as I search online for ways to tell This seems to be the same on debian, and osx, (I haven't got my windows build to complete yet). Thoughts on a valid development/debugging/testing installation process? -Mike |
|
The multiple package problem is likely caused by pip. I note that you have 23.1 installed, try upgrading. The version numbers for your editable installs are photonlibpy has a pinned dependency on Telling pip to ignore dependencies (via |
|
One last note, because I don't trust setuptools to do the right thing, I run We will not be using setuptools next year, so hopefully some of that will go away. |
|
My windows builds using x64 Native Tools Command Prompt fail this way: |
|
That error seems really familiar, I think we ran into it last year? I don't use Windows at all these days. I think you need to upgrade Visual Studio... according to this CD post you need 17.9 or later? |
|
This feels like a silly question, really bad planning on my part. But in debian bookworm I get: Debian bookworm doesn't support robotpy sim? |
|
WPILib does not support simulation on linux/aarch64 |
|
Actually, that doesn't seem to be true anymore, https://frcmaven.wpi.edu/ui/native/release/edu/wpi/first/halsim/halsim_gui/2025.3.2/ shows aarch64 packages. We would need to update mostrobotpy to build them and update the robotpy-meta constraints. |
|
I thought we do build it, it's just that by default robotpy-meta doesn't install the sim stuff on Linux aarch64? |
|
I have found a route to simulate a full robot with mostrobotpy editable on a OSX M3 Mac: First install Then: Python click module does most of the heavy lifting: Which runs these commands: Then: Things that were not obvious were:
|
Simulation should work fine on Linux aarch64. I've run the simulation for the examples on my Raspberry Pi 400. |
I created this issue to document where we are currently at: robotpy/robotpy-meta#33 |
|
Getting pretty close to the final product. It feels like I've resolved most of the review comments. Adding the docstrings uncovered some new conversations we should have about consistency in the API. The remaining tasks include: resolve the remaining todo's in the code, add unit tests, perhaps functional tests, test to see how how much slower the python version is, and test as a command2 TimedCommandRobot. @virtuald @auscompgeek, please look at the todo's in the code. |
virtuald
reviewed
Apr 30, 2025
| print(f"Default robotPeriodic() method...Override me!") | ||
| self._robotPeriodicHasRun = True | ||
|
|
||
| # todo why is this _simulationPeriodic in previous code? Can it be simulationPeriodic and still work? |
There was a problem hiding this comment.
I renamed it to _simulationPeriodic to discourage users from using it, and to put simulation stuff in physics.py instead. See also #130
There was a problem hiding this comment.
See also the place where the physics.py is hooked in: https://github.com/robotpy/pyfrc/blob/main/pyfrc/physics/core.py#L123
There was a problem hiding this comment.
okay changed to _simulationInit and _simulationPeriodic to get the physics code to work.
virtuald
reviewed
Apr 30, 2025
| periodic function. | ||
| """ | ||
|
|
||
| return self.loopStartTimeUs / 1e6 # units are seconds |
There was a problem hiding this comment.
I don't think this should return seconds? The documentation says it's the same units as GetFPGATimestamp, so it should do so?
There was a problem hiding this comment.
True. I don't know how I got lost in my translation, c++ is also microseconds: https://github.com/wpilibsuite/allwpilib/blob/main/wpilibc/src/main/native/cpp/TimedRobot.cpp#L102
|
Now that this test passes on my machine: subprojects/robotpy-wpilib/tests/test_timedrobot.py which just tests the scheduling math to schedule the future periodic callbacks, I'm fairly confident that the code is pretty close to working correctly. Perhaps @virtuald or @auscompgeek have suggestions on how to connect existing functional tests to this code? |
|
Well. There really aren't any python functional tests for this -- we're pretty light on testing since the wrapper stuff tends to Just Work if it compiles. The examples repo does full-robot tests -- eventually I'd like to run those tests as part of the CI for this repo, but haven't gotten around to it. It would probably make sense to port the tests in allwpilib from Java or C++ to python, assuming they exist. ChatGPT might give you a good first translation of them. |
|
Is there a spot in the existing robotpy code base that is equivalent to this line? In the existing robotpy code base is the thread a c++ thread or a python thread? I’m hoping to not have pave new ground in the threading model of the new test. |
|
Two more questions:
This is resolved on May 15, 2025. Resolution to this question: What was happening is that I was using this command line to rebuild mostrobotpy editable: What was happening was that the above command was putting a new a For record the problem: see |
|
Question two: (It works on my machine). For the online CI builds, what should I change to be able to find this module: -Thanks |
It should work. The error in CI appears to be that it can't find the reraise fixture, which is provided by pytest-reraise (add to tests/requirements.txt). |
|
@virtuald Would you approve the workflow. I'd like to see if it passes again. |
|
I added you as an outside collaborator, hopefully it doesn't ask me to approve it again :) |
|
An admin should probably just change that setting to only require approval for first-time contributors who are new to GitHub. |
auscompgeek
reviewed
May 17, 2025
| from wpilib.simulation._simulation import _resetWpilibSimulationData | ||
|
|
||
| try: | ||
| import commands2 |
|
I merged in my new build system changes, so please rebase on the current main and let me know how that works? Note that none of the vendor libraries have been pushed to pypi yet... there are |
|
Okay I will rebase on the current main. |
|
How would I install the artifacts from github the build: 1 - In MacOs? |
|
One thing I have noticed with |
|
I've pushed releases for rev, navx, urcl, and playingwithfusion -- so all of those should be available on PyPI within the next 20 minutes or so. To install releases from github, you can download the zip file from the action and unzip it somewhere. Then use Installation on RoboRIO is a bit more tricky because it depends on the robotpy package which enforces certain versions, but I should be able to push a robotpy release tonight. In theory, you should be able to copy the wheels to |
|
I haven't looked very hard at how TimedRobot works, so it's not clear to me whether that is expected behavior or not. I would assume that the timing is arbitrary, but I could see how one might think otherwise. |
MikeStitt
changed the title
MikeStitt
force-pushed
the
pyIterativeAndTimedRobot
branch
from
3eccb59 to
14c4096
Compare
|
@virtuald looking at I picked off these commands: It crashes here: Which is strange, because it seems like the online builds are working. |
|
I also tried this: |
|
Could use a suggestion as to what the best build editable process is now. |
|
You need to Additionally, due to pypackaging-native/pkgconf-pypi#70, you should do |
|
@virtuald Thanks. This now works. |
|
@virtuald this doesn't work yet: results in: |
|
Tracking the rdev issues at #172 |
|
Documenting a path that does allow me to these steps work: This didn't work because This didn't work because This didn't work because This works because This works because robotpy sim now uses the editable For reference |
|
I have this simulateable That can simulate with the C++ The With the legacy c++ Order of magnitude, it takes 81.28 microseconds to get to the first added task. and 91.37 microseconds to get to the 10th one. Switching to Order of magnitude the python On the MacBook Pro, concentrating on the time to get to the first extra task, the python version is, order of magnitude, 50 microseconds slower. If we can figure out how to run this code on a RoboRio, and the results are reasonable, I feel like this task is getting close to being complete. |
|
Next steps: |
|
@virtuald Could you look at https://github.com/robotpy/mostrobotpy/actions/runs/15209970105 ? The failure doesn't seem to relate to my changes. |
|
I was able to do some timing measurements on my roborio 2 by copying Order of magnitude, |
First version of iterativerobotpy.py Signed-off-by: Mike Stitt <mikestitt@peas.local> Simulates, but does not update robot position in simulation. Signed-off-by: Mike Stitt <mike@stitt.cc> Simulates, but does not update robot position in simulation. Signed-off-by: Mike Stitt <mike@stitt.cc> Only call the inits on a transition. Signed-off-by: Mike Stitt <mike@stitt.cc> This version does simulate in autonomous and teleop okay. Need to test timer math. Signed-off-by: Mike Stitt <mike@stitt.cc> Cleaning Up Signed-off-by: Mike Stitt <mike@stitt.cc> Cleaning up. Signed-off-by: Mike Stitt <mike@stitt.cc> Fix typos Signed-off-by: Mike Stitt <mike@stitt.cc> Correct periodS to be period, use from . to import Signed-off-by: Mike Stitt <mike@stitt.cc> Fix up watchdog interface. Signed-off-by: Mike Stitt <mike@stitt.cc> Cleaning up. Signed-off-by: Mike Stitt <mike@stitt.cc> Cleaning up code. Signed-off-by: Mike Stitt <mike@stitt.cc> Running through black Signed-off-by: Mike Stitt <mike@stitt.cc> Added docstrings, new todo's based upon docstring comparison. Signed-off-by: Mike Stitt <mike@stitt.cc> Added missing wpimath.units.second Signed-off-by: Mike Stitt <mike@stitt.cc> Fix typo, second to seconds Signed-off-by: Mike Stitt <mike@stitt.cc> use if rather than match for python 3.9 Signed-off-by: Mike Stitt <mike@stitt.cc> simulationPeriodic becomes _simulationPeriodic Signed-off-by: Mike Stitt <mike@stitt.cc> Get the physics engine to work. Signed-off-by: Mike Stitt <mike@stitt.cc> Fix units bug for getLoopStartTime. now returns microseconds. Signed-off-by: Mike Stitt <mike@stitt.cc> Make work with python 3.9 Signed-off-by: Mike Stitt <mike@stitt.cc> Add test of period math, and improve comments. Signed-off-by: Mike Stitt <mike@stitt.cc> fix black formatting Signed-off-by: Mike Stitt <mike@stitt.cc> Proof of Concept for TimedRobot Functional Tests Signed-off-by: Mike Stitt <mike@stitt.cc> WIP still sims. Signed-off-by: Mike Stitt <mike@stitt.cc> Improving simulation. Signed-off-by: Mike Stitt <mike@stitt.cc> Getting new tests to not break existing tests. Signed-off-by: Mike Stitt <mike@stitt.cc> Progress on tests. Signed-off-by: Mike Stitt <mike@stitt.cc> WIP - tests should pass. Signed-off-by: Mike Stitt <mike@stitt.cc> Debugging Signed-off-by: Mike Stitt <mike@stitt.cc> Run black, add to test requirements. Signed-off-by: Mike Stitt <mike@stitt.cc> Only call endCompetition once during tests. Signed-off-by: Mike Stitt <mike@stitt.cc> Adjusting tests. Signed-off-by: Mike Stitt <mike@stitt.cc> Add tests for addPeriodic, fix Override me text. Signed-off-by: Mike Stitt <mike@stitt.cc> Don't add the commans2 test. Signed-off-by: Mike Stitt <mike@stitt.cc> waitForNotifierAlarm provides loopStartTime, _getFPGATime for setNextStartTime Signed-off-by: Mike Stitt <mike@stitt.cc> Cleaning up tests. Signed-off-by: Mike Stitt <mike@stitt.cc> Consolidate tests in test_timedrobot.py Signed-off-by: Mike Stitt <mike@stitt.cc> Consolidate tests in test_timedrobot.py Signed-off-by: Mike Stitt <mike@stitt.cc> Fix build for python 3.9 Signed-off-by: Mike Stitt <mike@stitt.cc> Improve comments, siftup rather than pop and add. Signed-off-by: spiresFrc9106 <ccrobotics@grcatholiccentral.org> Improving readability of code. Signed-off-by: Mike Stitt <mike@stitt.cc>
MikeStitt
force-pushed
the
pyIterativeAndTimedRobot
branch
from
9b71587 to
3f3d25e
Compare
Signed-off-by: Mike Stitt <mike@stitt.cc>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A draft to get progress going.